Skip to content

Conversation

@ofirfarjun7
Copy link
Contributor

@ofirfarjun7 ofirfarjun7 commented Sep 17, 2025

What?

Pass UCP_DEVICE_FLAG_NODELAY flag when posting in UCP perftest.

Why?

Test hangs without it.

@ofirfarjun7 ofirfarjun7 changed the title Topic/pass nodelay flag Pass nodelay flag in ucp perftest Sep 17, 2025
Comment on lines 71 to 73
size_t m_size;
ucp_device_request_t m_requests[CAPACITY];
uint8_t m_pending[UCX_BITSET_SIZE(CAPACITY)];
uint8_t m_pending[UCX_BITSET_SIZE(CAPACITY)];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pls revert

status = ucp_device_put_single<level>(mem_list, mem_list_index, address,
remote_address, length, 0, &req);
remote_address, length,
UCP_DEVICE_FLAG_NODELAY, &req);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how did it pass CI?

Copy link
Contributor

@iyastreb iyastreb Sep 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, it works fine on my side without this flag

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we keep only this change and exclude formatting changes?

Copy link
Contributor Author

@ofirfarjun7 ofirfarjun7 Sep 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did it worked for you with two nodes? With single node & cudaipc it will not go throw NIC so it will work without DB. (Maybe this is why CI passed?)

I can just add the flag and revert the rest. We need it to measure perf so I don't want to wait for PR with many changes that require review

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, I haven't tested with two nodes
Thanks for reducing the change

yosefe
yosefe previously approved these changes Sep 18, 2025
Copy link
Contributor

@iyastreb iyastreb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems this PR only sets a flag, which I can probably just add in #10893
All the formatting changes are redundant, most of these functions are changed anyway in the PR above, and it will just cause merging issues

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants